[MXNET-400] support string type for kvstore key in cpp-package #10792

nihui · 2018-05-03T04:45:38Z

Description

implement string key variant for KVStore in cpp-package

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

nswamy · 2018-08-09T23:11:55Z

@mxnet-label-bot please add [kvstore, pr-awaiting-review]

lupesko · 2018-08-21T06:19:30Z

@nihui can you please look into the failing CI tests? thanks for your contribution!

lupesko · 2018-09-05T06:25:55Z

@nihui bounce...

nihui · 2018-09-06T14:13:32Z

Hi
I am traveling now.
I will study this issue later next week.

stu1130 · 2018-09-21T22:48:41Z

@nswamy would you mind to take a look at it! Thanks

vandanavk · 2018-09-24T21:11:44Z

@leleamol for review

vrakesh · 2018-10-09T07:29:48Z

@leleamol Requesting a review on this PR.

Roshrini · 2018-10-22T16:56:38Z

@rahul003 @cjolivier01 Can you please review this PR? Thanks

ankkhedia · 2018-10-30T18:52:50Z

@leleamol @rahul003 @nswamy ping!
Could you please review this PR?

rahul003

Looks good to me, except the fact that it doesn't have tests. Could you please add couple of cpp tests? You can look at these for examples : https://github.com/apache/incubator-mxnet/blob/master/tests/cpp/kvstore/gpu_topology_test.cc

nihui · 2018-11-15T11:33:30Z

new kvstore testcase added

stu1130 · 2018-11-20T20:47:11Z

Thanks @nihui for adding the test. @rahul003 could you please take a look at it?

leleamol · 2018-11-27T18:34:47Z

cpp-package/example/test_kvstore.cpp

+
+  std::string key = "singlekeytest";
+
+  NDArray result(Shape(4), Context::cpu());


Can you add similat test for gpu context? Or make this test configurable to run on cpu as well as gpu.

leleamol · 2018-11-27T18:39:05Z

cpp-package/example/test_kvstore.cpp

+    ret += (results[1].GetData()[j] == (grads[1].GetData()[j] + grads[2].GetData()[j])) ? 0 : 1;
+  }
+
+  return ret;


Thanks for adding the unit test. However, it would be helpful if you can add some logging. Especially, for the cases when the test fails.

lupesko · 2018-12-11T00:12:54Z

@nihui thanks for your contribution! can you please look into the comments and failing builds?

Roshrini · 2018-12-20T21:21:58Z

@nihui Can you please address comments and rebase this PR so that we can move forward with it?

Roshrini · 2019-01-02T19:22:08Z

@nihui Could you please rebase this PR?
@mxnet-label-bot Update [pr-awaiting-testing, KVStore]

stu1130

Thanks for your contribution @nihui
The rest part LGTM
Could you rebase the PR?

stu1130 · 2019-01-15T21:27:56Z

cpp-package/example/test_kvstore.cpp

+  // initialize data
+  std::vector<NDArray> datas(2);
+  datas[0] = NDArray({0.f, 2.f, -3.12f, 4.f}, Shape(4), Context::cpu());
+  datas[1] = NDArray({0.8f, -2.f, 6.6f, 77.f}, Shape(4), Context::cpu());


nit

Suggested change

datas[1] = NDArray({0.8f, -2.f, 6.6f, 77.f}, Shape(4), Context::cpu());

data[1] = NDArray({0.8f, -2.f, 6.6f, 77.f}, Shape(4), Context::cpu());

stu1130 · 2019-01-15T21:55:13Z

cpp-package/example/test_kvstore.cpp

+  KVStore::SetType("local");
+
+  int ret1 = test_single_key();
+  int ret2 = test_multiple_key();


would it be better if adding some info to know which test case is falling?

sandeep-krishnamurthy · 2019-01-25T19:28:15Z

@nihui - Very useful contribution. Thanks. Last few things and we are good to go here:

Please address test case comment by @leleamol
Please make nit changes suggested by @stu1130
Please rebase and resolve conflicts

vandanavk · 2019-02-05T23:02:14Z

@mxnet-label-bot update [KVStore, pr-awaiting-response]

ankkhedia · 2019-02-14T22:10:18Z

@nihui Thanks for your contribution! COuld you look into the comments added by @sandeep-krishnamurthy

anirudhacharya · 2019-03-01T22:24:46Z

@nihui ping to update your PR with extra tests.

lupesko · 2019-03-18T23:46:37Z

@nihui - since there was no feedback/follow up on feedback since May 2018, we should probably close the PR. You can always re-open if done in error.
@Roshrini can you please help by closing this?

Roshrini · 2019-03-19T16:41:27Z

Hi @nihui, Thank you so much for working on this. This PR is open since long time with no action. I am closing it for now. Please do reopen if closed in error and we can help you to take it to completion.

nihui · 2019-03-29T09:40:29Z

hello, I'm unable to reopen a this closed PR. "Reopen" option is missing here.

TaoLv · 2019-03-29T09:57:37Z

Reopen for you @nihui

* support string type for kvstore key in cpp-package * make lines short * fix build * add kvstore testcase * no rand() use * fix cpplint sanity check * support string type for kvstore key in cpp-package * make lines short * fix build * print error log

nihui · 2019-04-01T10:38:25Z

gpu unittest added, which is disabled automatically on cpu-only machine
log verbose error on fail

piyushghai · 2019-04-02T21:35:39Z

@leleamol @rahul003 @nswamy ping!
Could you please review this PR?

wkcn

Thanks for your contribution! LGTM.

cpp-package/example/test_kvstore.cpp

cpp-package/include/mxnet-cpp/kvstore.h

cpp-package/example/test_kvstore.cpp

wkcn · 2019-04-09T07:56:18Z

Merged. Thanks for your contribution!

…e#10792) * Kvstore strkey (apache#2) * support string type for kvstore key in cpp-package * make lines short * fix build * add kvstore testcase * no rand() use * fix cpplint sanity check * support string type for kvstore key in cpp-package * make lines short * fix build * print error log * Update test_kvstore.cpp * update * add gpu unittest * check gpu count * fix sanity check

nihui changed the title ~~[WIP] support string type for kvstore key in cpp-package~~ [MXNET-400] support string type for kvstore key in cpp-package May 4, 2018

marcoabreu added KVStore pr-awaiting-review PR is waiting for code review labels Aug 9, 2018

nihui requested a review from nswamy as a code owner September 11, 2018 04:57

rahul003 suggested changes Oct 31, 2018

View reviewed changes

nihui requested a review from marcoabreu as a code owner November 15, 2018 11:04

leleamol suggested changes Nov 27, 2018

View reviewed changes

marcoabreu added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-awaiting-review PR is waiting for code review labels Jan 2, 2019

stu1130 approved these changes Jan 15, 2019

View reviewed changes

marcoabreu added pr-awaiting-response PR is reviewed and waiting for contributor to respond and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Feb 5, 2019

Roshrini closed this Mar 19, 2019

TaoLv reopened this Mar 29, 2019

TaoLv requested review from anirudh2290, eric-haibin-lin, gigasquid, iblislin, sergeykolychev, szha and yzhliu as code owners March 29, 2019 09:57

nihui changed the title ~~[MXNET-400] support string type for kvstore key in cpp-package~~ [WIP] [MXNET-400] support string type for kvstore key in cpp-package Mar 29, 2019

Kvstore strkey (#2)

24ca621

* support string type for kvstore key in cpp-package * make lines short * fix build * add kvstore testcase * no rand() use * fix cpplint sanity check * support string type for kvstore key in cpp-package * make lines short * fix build * print error log

nihui force-pushed the kvstore-strkey branch from 6a5b2d3 to 24ca621 Compare March 29, 2019 10:43

nihui added 4 commits March 29, 2019 22:34

Update test_kvstore.cpp

803587f

update

f850861

add gpu unittest

c5cac66

check gpu count

edf6554

nihui changed the title ~~[WIP] [MXNET-400] support string type for kvstore key in cpp-package~~ [MXNET-400] support string type for kvstore key in cpp-package Apr 1, 2019

fix sanity check

b64d58c

wkcn approved these changes Apr 8, 2019

View reviewed changes

wkcn added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-response PR is reviewed and waiting for contributor to respond labels Apr 8, 2019

wkcn merged commit 74e71e9 into apache:master Apr 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MXNET-400] support string type for kvstore key in cpp-package #10792

[MXNET-400] support string type for kvstore key in cpp-package #10792

nihui commented May 3, 2018 •

edited

Loading

nswamy commented Aug 9, 2018

lupesko commented Aug 21, 2018

lupesko commented Sep 5, 2018

nihui commented Sep 6, 2018

stu1130 commented Sep 21, 2018

vandanavk commented Sep 24, 2018

vrakesh commented Oct 9, 2018

Roshrini commented Oct 22, 2018

ankkhedia commented Oct 30, 2018

rahul003 left a comment

nihui commented Nov 15, 2018

stu1130 commented Nov 20, 2018

leleamol Nov 27, 2018

leleamol Nov 27, 2018

lupesko commented Dec 11, 2018

Roshrini commented Dec 20, 2018

Roshrini commented Jan 2, 2019

stu1130 left a comment •

edited

Loading

stu1130 Jan 15, 2019

stu1130 Jan 15, 2019

sandeep-krishnamurthy commented Jan 25, 2019

vandanavk commented Feb 5, 2019

ankkhedia commented Feb 14, 2019

anirudhacharya commented Mar 1, 2019

lupesko commented Mar 18, 2019

Roshrini commented Mar 19, 2019

nihui commented Mar 29, 2019

TaoLv commented Mar 29, 2019

nihui commented Apr 1, 2019

piyushghai commented Apr 2, 2019

wkcn left a comment

wkcn commented Apr 9, 2019


		std::string key = "singlekeytest";

		NDArray result(Shape(4), Context::cpu());

	datas[1] = NDArray({0.8f, -2.f, 6.6f, 77.f}, Shape(4), Context::cpu());
	data[1] = NDArray({0.8f, -2.f, 6.6f, 77.f}, Shape(4), Context::cpu());

[MXNET-400] support string type for kvstore key in cpp-package #10792

[MXNET-400] support string type for kvstore key in cpp-package #10792

Conversation

nihui commented May 3, 2018 • edited Loading

Description

Checklist

Essentials

nswamy commented Aug 9, 2018

lupesko commented Aug 21, 2018

lupesko commented Sep 5, 2018

nihui commented Sep 6, 2018

stu1130 commented Sep 21, 2018

vandanavk commented Sep 24, 2018

vrakesh commented Oct 9, 2018

Roshrini commented Oct 22, 2018

ankkhedia commented Oct 30, 2018

rahul003 left a comment

Choose a reason for hiding this comment

nihui commented Nov 15, 2018

stu1130 commented Nov 20, 2018

leleamol Nov 27, 2018

Choose a reason for hiding this comment

leleamol Nov 27, 2018

Choose a reason for hiding this comment

lupesko commented Dec 11, 2018

Roshrini commented Dec 20, 2018

Roshrini commented Jan 2, 2019

stu1130 left a comment • edited Loading

Choose a reason for hiding this comment

stu1130 Jan 15, 2019

Choose a reason for hiding this comment

stu1130 Jan 15, 2019

Choose a reason for hiding this comment

sandeep-krishnamurthy commented Jan 25, 2019

vandanavk commented Feb 5, 2019

ankkhedia commented Feb 14, 2019

anirudhacharya commented Mar 1, 2019

lupesko commented Mar 18, 2019

Roshrini commented Mar 19, 2019

nihui commented Mar 29, 2019

TaoLv commented Mar 29, 2019

nihui commented Apr 1, 2019

piyushghai commented Apr 2, 2019

wkcn left a comment

Choose a reason for hiding this comment

wkcn commented Apr 9, 2019

nihui commented May 3, 2018 •

edited

Loading

stu1130 left a comment •

edited

Loading